-
Notifications
You must be signed in to change notification settings - Fork 6.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[avro-cpp] Fix static build #32854
[avro-cpp] Fix static build #32854
Conversation
Usage passed on x64-osx with following demo code cmake_minimum_required(VERSION 3.15)
project(test_avro_cpp)
set(CMAKE_CXX_STANDARD 17)
find_package(unofficial-avro-cpp CONFIG REQUIRED)
add_executable(test_avro_cpp main.cpp cpx.hh)
set_source_files_properties("cpx.hh" PROPERTIES GENERATED TRUE)
find_program(ACROGENCPP avrogencpp REQUIRED)
add_custom_command(
OUTPUT "cpx.hh"
COMMAND
"${ACROGENCPP}"
"-i"
"cpx.json"
"-o"
"cpx.hh"
"-n"
"c"
DEPENDS
"cpx.json"
WORKING_DIRECTORY
"${CMAKE_CURRENT_SOURCE_DIR}"
)
target_link_libraries(test_avro_cpp PRIVATE unofficial::avro-cpp::avrocpp) {
"type": "record",
"name": "cpx",
"fields" : [
{"name": "re", "type": "double"},
{"name": "im", "type" : "double"}
]
} #include <iostream>
#include "cpx.hh"
#include "avro/Encoder.hh"
#include "avro/Decoder.hh"
int main() {
std::unique_ptr<avro::OutputStream> out = avro::memoryOutputStream();
avro::EncoderPtr e = avro::binaryEncoder();
e->init(*out);
c::cpx c1{};
c1.re = 1.0;
c1.im = 2.13;
avro::encode(*e, c1);
std::unique_ptr<avro::InputStream> in = avro::memoryInputStream(*out);
avro::DecoderPtr d = avro::binaryDecoder();
d->init(*in);
c::cpx c2{};
avro::decode(*d, c2);
std::cout << '(' << c2.re << ", " << c2.im << ')' << std::endl;
return 0;
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the existing usage
needs to be rectified (or removed).
It doesn't consistently look for debug and release libs.
It only deals with lib acrocpp. It doesn't carry any link libraries for static linkage. (This doesn't mean that the port wouldn't support static linkage. It just means that users are required to have more knowledge about the package.)
For static linkage, the port could benefit from exporting unofficial cmake config.
@dg0yt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of new comments... Some of the problems are more related to the poor original state of the port/patch, not to the contribution in this PR which is a signifcant progress.
@dg0yt |
Now that the patch is smaller and all your suggestions are dealt with, please review again. |
./vcpkg x-add-version --all
and committing the result.Fix #32848
Also fixed these in ci baseline.